Repository navigation
Prune affix template partials whose required slot cannot find its material - #575
johnml1135 wants to merge 1 commit into
Conversation
…erial An analysis template partial now stops as soon as a required slot can no longer find the segments its affix inserts, instead of paying for every optional slot before the required one. The test is used only while every slot walked since is a pure copy-and-insert rule, so parses do not change. A count-only shadow mode and a null-by-default check observer let a harness verify soundness and cost on real grammars. Closes #561 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
==========================================
+ Coverage 74.57% 74.65% +0.07%
==========================================
Files 453 454 +1
Lines 38342 38573 +231
Branches 5284 5338 +54
==========================================
+ Hits 28593 28796 +203
- Misses 8583 8601 +18
- Partials 1166 1176 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (RequiredSlotCannotUnapply(input, _rules.Count - 1, ref fired)) | ||
| { | ||
| if (_morpher.TraceManager.IsTracing) | ||
| _morpher.TraceManager.EndUnapplyTemplate(_template, input, false, FailureReason.None); |
There was a problem hiding this comment.
Minor: F1. Tracing loses the failure reason: with the prune on by default, a pruned template now traces as TemplateAnalysisInput then a null output with FailureReason.None, and the per-rule not-unapplied entries that showed which slot failed are gone. Apply_Tracing_ShowsAPrunedTemplateAsAFailedOutput locks this in for sagufz. Skip the prune while TraceManager.IsTracing, or give it its own FailureReason.
| // matched against the segments the matcher considers, so neither can be tested here. | ||
| private static void AddRequirement(List<FeatureStruct> requirements, FeatureStruct fs) | ||
| { | ||
| if (fs.HasVariables || (FeatureSymbol)fs.GetValue(HCFeatureSystem.Type) != HCFeatureSystem.Segment) |
There was a problem hiding this comment.
Minor: F2. A grammar that compiled before can now throw while the Morpher is built: fs.GetValue(HCFeatureSystem.Type) throws ArgumentException when Type is absent, and so do constraint.Type() in HasSegmentConstraint/MinSegments. A NaturalClass given an already-frozen FS never gets Type (NaturalClass.cs:9). Use TryGetValue and treat a missing Type as unprunable and impure. Unverified: not run, and the XML loader may never produce such an FS.
| /// Whether affix template walks drop partials in which a required slot can no longer unapply because | ||
| /// the material it inserts is absent. See <see cref="RequiredSlotMaterial"/>. | ||
| /// </summary> | ||
| internal RequiredSlotLookaheadMode RequiredSlotLookahead { get; set; } = RequiredSlotLookaheadMode.On; |
There was a problem hiding this comment.
Minor: F3. Consumers of the package have no way to turn the prune off: it changes analysis for every one of them, defaults to On, and the switch is internal. If the purity argument misses a case in some grammar, parses are lost silently, with no public way to switch it off or confirm the cause, unlike DeletionReapplications beside it. Make it public, or say why not.
| /// Called after each required-slot check that unified, with the number of unifications it performed. | ||
| /// Null unless a diagnostic sets it; it may be called from several threads. | ||
| /// </summary> | ||
| internal Action<long> RequiredSlotCheckObserver { get; set; } |
There was a problem hiding this comment.
FYI: F4. RequiredSlotCheckObserver is never set in the product or tests, and Shadow mode with its string-kinded callback ("fired", "violated", "completed-under-fired") is measurement scaffolding. It threads an int[] through every walk tuple. Consider moving it to the research branch, or keeping it test-only.
| return true; | ||
| } | ||
|
|
||
| private static bool IsPresent(FeatureStruct required, Shape shape, ref long unifications) |
There was a problem hiding this comment.
FYI: F5. IsPresent rescans the whole shape for each distinct constraint, and the known/present cache lives for one CannotUnapply call, so a constraint shared by several slots is rescanned for each one on every partial. One pass per shape across all pending slots may recover some of Amharic's net loss. Unverified: not measured.
| } | ||
|
|
||
| // An optional slot whose rule inserts "z" after the copied parts; the word never contains the t and i | ||
| // that the required inner slot inserts, so a pure rule lets the walk prune at entry, before the optional rule is tried, and an impure one does not. |
There was a problem hiding this comment.
FYI: F6. This comment line is about 150 columns, which is over the 120-column limit in .claude/skills/code-comments/SKILL.md that --agent-strict enforces. Rewrap it.
| /// allomorph of some rule in the slot must have every inserted segment unify with a segment of the shape. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The tables are built once from the template and depend only on the grammar. |
There was a problem hiding this comment.
FYI: F7. "depend only on the grammar" uses a term AGENTS.md reserves for the whole configuration, and it overstates the dependency: the tables depend only on this template's slot rules and their allomorphs. Say that.
|
Verdict: approve with fixes. Most important: F2 (Important): a grammar that compiled before can now fail while the Counts: 0 Critical, 3 Important (F1-F3), 4 Low (F4-F7). Ran: Not verified: F2 is reasoned from Public API, target framework, package, or parity contract changed: None verified. The new members are Findings: F1 new, F2 new and unverified, F3 new, F4 new, F5 new and unverified, F6 new, F7 new. Reviewed at 0bf9e64 |
Quick summary
HermitCrab's affix-template walk now drops a partial as soon as a required slot can no longer find the segments its affix inserts. Parses are unchanged; the doomed partials stop paying for every optional slot between them and the required one. Only
AnalysisAffixTemplateRulechanges, and a slot is tested only while every slot walked since is a pure copy-and-insert rule.Net cost is not always a win: counted in feature-structure unification calls over the whole parse (look-ahead included), Mbugwe (+0.33%) and conformance (+0.14%) are break-even, Sena (+1.6%), Aweti (+4.5%) and the stress grammar (+37.9%) do less work, and Amharic (-0.63%) does more work (net negative, 24 words).
Where to look
RequiredSlotMaterialclass remarks give the argument;Apply_ImpureSlotBetween_DoesNotPruneForTheInnerRequiredSlotis red when the purity gate is removed.*-quantified or alternation copied part,ModifyFromInputslots, null affixes, Optional shape nodes,Optionaltoggled after compile -- unit tests inAnalysisAffixTemplateRuleTests, each asserting identical parses with the modeOffandOn.Apply_Parallel_ReturnsTheSameParsesAsSequential,Apply_Tracing_ShowsAPrunedTemplateAsAFailedOutput.Morpher.RequiredSlotShadow(count-only shadow mode) andRequiredSlotCheckObserver(null by default) exist so a harness can check soundness and cost on real grammars; nothing in the hot path takes a lock or an atomic.Deliberately not included
internal.Validation
dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release-- 168 passed, 0 failed (master: 151 passed).FeatureStructcall counts, census columns, the stress fixture) are not in this PR; they live only inresearch/hc-rsl-census@6ad11702.dotnet csharpier check .clean;pwsh ./scripts/comment-hygiene.ps1 origin/masterclean.Issue / porting context
Closes #561
🤖 Generated with Claude Code
This change is